Support overriding theme through URL search parameter #7791
Conversation
Signed-off-by: Joshua Li <joshuali925@gmail.com>
joshuali925
requested review from
ananzh,
kavilla,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi and
sejli
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7791 +/- ##
==========================================
+ Coverage 63.81% 64.30% +0.48%
==========================================
Files 3656 3674 +18
Lines 81205 81135 -70
Branches 12949 12926 -23
==========================================
+ Hits 51823 52173 +350
+ Misses 26202 25751 -451
- Partials 3180 3211 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Does it matter that #5652 is not in 2.x? |
| /** @type {'browser' | 'light' | 'dark' | null} */ | ||
| var urlThemeParam = null; | ||
| try { | ||
| urlThemeParam = new URLSearchParams(window.location.search).get('theme'); |
There was a problem hiding this comment.
if this parameter only impacts dark/light mode, can we name it after that and not use theme?
There was a problem hiding this comment.
yes it's like 'colorscheme' vs 'background', and "theme" only impacts background. i thought about something like darkMode, but it also needs to support "browser". do you have any recommendation?
| if (!urlThemeParam && window.location.hash.includes('?')) { | ||
| urlThemeParam = new URLSearchParams(window.location.hash.split('?')[1]).get('theme'); |
There was a problem hiding this comment.
we specify urls params in the url fragment?
There was a problem hiding this comment.
we'll take it if it's in the hash. when hashrouter is used i think many parts of dashboards use hash for url params
There was a problem hiding this comment.
In a future enhancement, we MIGHT be having an interceptor on the server side to handle language and theme changes among others. After intercepting, it will clean the URL and redirect to the destination without the lang and theme. If we look into the hash here, we cannot unlook at it later and that would make the transition challenging if not impossble without a breaking change.
IMO, we should drop this piece and stick to the one above.
Signed-off-by: Joshua Li <joshuali925@gmail.com>
|
this is no longer needed |
Description
This PR allows theme to be overridden by search parameter.
themeisdark, OSD sets theme to dark.themeisbrowser, OSD sets theme to match browser.themeislight, OSD sets theme to light.themeis absent or any other value, OSD behavior does not change.Theme can be a search param in either
location.searchorlocation.hash. For examplehttp://localhost:5601/app/home#/?theme=darkhttp://localhost:5601/app/management?theme=lighthttp://localhost:5601/app/home?theme=browser#/?theme=darkThis override will only be in-effect if the parameter is present in the URL. It only changes the actual theme and does not change theme selection on UI. This is more for the
embeded use case and not a user facing feature.This PR depends on #5652. The handlebars files don't seem to have tests, will add tests for 5652 in general later.
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration